fix(hotkeys): make copy line up/down hotkeys editable#10241
fix(hotkeys): make copy line up/down hotkeys editable#10241Abdulrehman-PIAIC80387 wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
All contributors have signed the CLA ✍️ ✅ |
Copy line(s) up/down were listed in the Keyboard Shortcuts panel but marked `editable: false`, because they were served by CodeMirror's built-in `defaultKeymap` and so never consulted marimo's hotkey config. Route both commands through the existing override mechanism already used by `toggleComment`/`toggleBlockComment`: add them to OVERRIDDEN_COMMANDS so the hardcoded bindings are filtered out of the default keymap, then re-bind them in `overrideKeymap` using the configured hotkey. Default keybindings are unchanged. Closes marimo-team#7056
7c1ea93 to
f8e6a53
Compare
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
There was a problem hiding this comment.
Pull request overview
Enables user customization of the editor hotkeys for CodeMirror’s built-in “copy line up/down” commands by filtering their hardcoded default bindings and re-binding them through marimo’s hotkey provider, and by marking these actions as editable in the shortcuts UI.
Changes:
- Remove the
editable: falserestriction forcell.copyLineUp/cell.copyLineDownso they can be customized in the Keyboard Shortcuts panel. - Override CodeMirror’s
defaultKeymapbindings forcopyLineUp/copyLineDownand re-bind them usingkeymap.getHotkey(...)so user overrides take effect. - Add a focused unit test to assert that user overrides (e.g.,
Ctrl-d) propagate into the resulting CodeMirror keymap bindings.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| frontend/src/core/hotkeys/hotkeys.ts | Makes “Copy line(s) up/down” editable by removing the explicit non-editable flags. |
| frontend/src/core/codemirror/keymaps/keymaps.ts | Filters out CodeMirror’s hardcoded bindings for copyLineUp/down and re-binds them via marimo’s configured hotkeys. |
| frontend/src/core/codemirror/keymaps/tests/keymaps.test.ts | Adds a regression test ensuring hotkey overrides actually reach the CodeMirror keymap for copyLineUp/down. |
| "cell.copyLineUp": { | ||
| name: "Copy line(s) up", | ||
| group: "Editing", | ||
| key: "Alt-Shift-ArrowUp", | ||
| editable: false, | ||
| }, |
|
@Abdulrehman-PIAIC80387 Can you please attach a demo video. I'll be happy to approve after that |
|
Hi @kirangadhave — added a demo video to the PR description showing "Copy line(s) down" being made editable, rebound to a new key, and that key duplicating a line in a cell. Further comments welcome. |
This pull request was authored by a coding agent.
📝 Summary
Closes #7056
Copy line(s) up/Copy line(s) downappear in the Keyboard Shortcuts panel but are markededitable: false, so the panel advertises two rows it silently refuses to customize.That flag was a deliberate stopgap in #6843, which said as much:
The commands are served by CodeMirror's built-in
defaultKeymap, which hardcodesShift-Alt-Arrow, so marimo's hotkey config never reached them. @mscolnick invited a follow-up FR for this in #6338 ("we can look into making it editable. mind filing a FR?"), which became #7056.🔍 Description of Changes
Uses the override mechanism already established in
keymaps.tsfortoggleComment/toggleBlockComment— no new machinery:frontend/src/core/codemirror/keymaps/keymaps.ts— addcopyLineUp/copyLineDowntoOVERRIDDEN_COMMANDS(filters the hardcoded bindings out of the default keymap), then re-bind them inoverrideKeymap()viakeymap.getHotkey(...)so they follow user config.frontend/src/core/hotkeys/hotkeys.ts— removeeditable: falsefrom both entries.Default keybindings are unchanged — only the ability to reconfigure them is added. Applies to both the
defaultandvimpresets, sincekeymapBundleinstallsoverrideKeymapfor each.Both directions are included because the issue asks for both and they share one mechanism.
🎥 Demo
Recorded against a local dev build on this branch.
The clip shows, in order:
mainit is greyed out / non-editable).marimo-shortcut-demo.mp4
🧪 Testing
make fe-check— cleanpnpm vitest run src/core/codemirror src/core/hotkeys— 660 passed, 1 skipped (47 files)setup.test.tsis unchanged at 18 duplicates — no new conflicts.The pre-existing generic assertions in
keymaps.test.ts(overrideKeymap().length === OVERRIDDEN_COMMANDS.size, and no overridden command surviving in the filtered keymap) picked up the two new commands automatically.Added a test asserting an override to
Ctrl-Dactually reaches the keymap — the exact rebinding @mullimanko asked for. It guards the user-facing behavior rather than the plumbing: reverting theoverrideKeymaphalf while leavingeditableon would still pass the generic tests, but fails this one.📋 Pre-Review Checklist
✅ Merge Checklist